-
Notifications
You must be signed in to change notification settings - Fork 0
GitHub in JS #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GitHub in JS #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❇️ CodePress Review Summary
👋 Hey team,
Overall the changes look solid, but I spotted 11 must-fix issues and left 0 helpful notes inline.
Here's the quick rundown:
❌ Decision: REQUEST CHANGES
The SWC config injection uses new Function (unsafe-eval), which will break under common CSPs, and the wrapper logic redeclares an existing default export identifier (export default App), causing a correctness error. These are blocking issues that must be fixed before merge.
🚧 Needs a bit of love
The most critical issue is the pervasive use of new Function/eval to inject config and metadata, which violates CSP (unsafe-eval) and will throw at construction time, causing runtime failures before any inner try/catch can run. Replace these with static, AST-generated statements that guard on typeof window !== 'undefined' and assign to window.CODEPRESS_CONFIG via Object.assign, inserted appropriately after the directive prologue. Config injection is also incorrectly gated by a shared, never-reset GLOBAL_ATTRIBUTES_ADDED flag, leading to skipped or nondeterministic injection across builds; use a dedicated per-transform/per-build flag instead. Additionally, the default-export wrapper logic can introduce duplicate bindings or name collisions (e.g., reusing App), so emit an anonymous default wrapper or a distinct identifier only when an original name exists, and ensure the esbuild plugin injects the config once per build rather than into every module to avoid bundle bloat and repeated work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR centralizes repo/branch configuration into window.CODEPRESS_CONFIG across SWC, Babel, and esbuild integrations, and refactors HMR to wrap the default export component with __CPRefreshProvider instead of rewriting return sites. It also updates the internal CodePress review workflow to v4 and bumps the package version to 0.9.0.
Reviewed areas
- SWC transform (config injection and CPRefreshProvider wrapping)
- Babel and esbuild config injection paths
- Removal of DOM attribute-based repo/branch propagation
- GitHub Actions workflow and package metadata
Blocking issues (🔴 REQUIRED)
- SWC: inject_config and inject_metadata_map both use new Function(...) to eval generated code, which violates CSP (requires 'unsafe-eval') and will be blocked in many production environments. This can break module evaluation and leave window.CODEPRESS_CONFIG unset. These sites must emit direct AST-based statements instead of eval.
- SWC: inject_config is gated by and mutates GLOBAL_ATTRIBUTES_ADDED, conflating config injection with historical DOM-attribute state. This global flag is never reset and may cause config injection to be skipped across modules/builds.
- SWC: The RefreshProviderWrapper wrapper currently reuses the original identifier name in some ExportDefaultExpr cases and names the wrapper App when no original_ident is present. Both patterns can introduce duplicate bindings or name collisions in the module scope.
- Esbuild: The module-level window.CODEPRESS_CONFIG snippet is prefixed into every transformed module, causing bundle bloat and repeated runtime work instead of a single, build-scoped injection.
These are substantial security, correctness, and performance concerns for consumers with CSP enabled or large codebases. Once the eval usage is removed, the config gating is decoupled from GLOBAL_ATTRIBUTES_ADDED (or otherwise made robust), wrapper naming is made collision-safe, and esbuild’s config injection is de-duplicated or centralized, the overall direction of the PR looks solid.
Inject window config and refactor HMR wrapping
This PR moves repo/branch propagation from DOM attributes to a global window.CODEPRESS_CONFIG and refactors the HMR provider injection to wrap the default export component externally.
Key Changes:
Review Notes: